This add the property @test.fixme in CI visibility, this is very useful to create dashboards

❓ question: What is the reasoning behind this version bump?‏

Older version of firefox don't work with this version of playwright.

The version we used to test was already chosen arbitrary and did not reflect the oldest version supported.
For unit tests however we test on the oldest browser version we support

Fore context: Firefox 91 was chosen because it was a ESR version. We were aware that using playwright would imply bumping a few browser versions, so using v119 looks fine to me.

❓ question: ‏What qualifies a function to be in TestContext vs used standalone by invoking it with page/browserContext?

I see we have deleteAllCookies in TestContext, but setCookie as standalone. I feel like we would scale more with standalone functions, but at the same time things might be a bit more verbose. WDYT?

Good question! Playwright encourage to use fixtures, so adding a function to TestContext fit well with Playwright style.

However, I think there is room for both:

• For heavily used functions, such as flushEvents, withBrowserLogs, I think it's convenient to not have to pass page or browserContext all the time.
• For function that are less often used or used only in one test file, then I think it might be better to have standalone function.

I think deleteAllCookies is not actually a good example as it's just doing browserContext.clearCookies() so we might use that directly.

At the end, it's a bit inconsistent in this PR as multiple people have migrated tests without clear direction.

❓ question: ‏can we have a test that doesn't rely on the environment? Maybe using process.argv?

we certainly could detect browserstack differently here, but BS_USERNAME and BS_ACCESS_KEY are populated as env variable anyway and used for unit-bs job too.

I thought it would be convenient to use that as a signal test are running on BS.

🥜 nitpick: ‏

if (this.logs.some((log) => log.message.includes('Ignoring unsupported entryTypes: ')) {

Unfortunately, we need to actually filter out these warning message, otherwise a bunch of test are failing because they expect logs to be a certain length.

Ultimately this need to be fixed in the observer as this message is shown to customers (but this seems out of scope for this PR)

nevermind, I guess it's more readable, for little performance overhead!

🥜 nitpick: ‏use addTag here

🥜 nitpick: ‏

👏 praise: ‏really nice to be able to locate elements within shadow dom

💬 suggestion: ‏we should have a single way to identify which browser is being used.

Previously, we had the getBrowserName function that returned a normalized browser name. It returned a strongly typed constant string, making it easy to understand what to expect. Now we have (at least) three ways of getting the browser name:

• testContext.browserName === 'chromium'
• testContext.browserName.includes("firefox")
• test.info().project.metadata.sessionName === 'Edge'

In the same spirit, I think testContext.browserName should:

• be strongly typed, with constant strings instead of arbitrary strings
• support all browsers (firefox, edge, ...)
• normalize the names (ex: playwright-firefox should be normalized to firefox)

And we should avoid having to pick into test.info() to get that information

Actually it is normalized (here) because browserName is a playwright property, and it's type is 'chromium' | 'firefox' | 'webkit'

About testContext.browserName.includes("firefox"), it is a mistake, I'm fixing it to testContext.browserName === 'firefox' 👌

However, Edge browsers have chromium for browserName, but the viewport issue is really on Edge, not on all chromium browsers, hence this ugly trick. 😞
I guess we could normalize browserName in TestContext differently to playwright's meaning. That would not be too much of a hassle. wdyt?

differently to playwright's meaning

Yes, I think it would be good!

I don't think this needs to be within the e2e test suite. If it makes things more complicated, we could imagine moving it somewhere else (in a standalone script).

I think it's actually more convenient to have it run in the e2e, we avoid setting up a separate job, script, test runner, ...

💬 suggestion: ‏Don't use new Promise when you already have promises:

const rawHeaders = await page.evaluate(
        () =>
            window
              .fetch('/headers', {
                headers: [
                  ['x-foo', 'bar'],
                  ['x-foo', 'baz'],
                ],
              })
              .then((response) => response.text())
              .catch(new Error('Fetch request failed!'))
      )

(Same suggestion applies to other new Promise in that file)